fix: merge runner config into workflow-triggered jobs#1098
Conversation
Workflow runs were dropping serverUrl/apiKey from the JobAgent runner row, so argo-workflow dispatch failed even though the same credentials work for the deployment path (which already merges in jobeligibility).
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe changes introduce configuration merging when dispatching workflow-triggered jobs. The Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 21 minutes and 39 seconds.Comment |
There was a problem hiding this comment.
Pull request overview
Fixes workflow-triggered job dispatch so the persisted job_agent_config includes the selected runner’s shared credentials (e.g., Argo Workflows serverUrl/apiKey) by merging the runner row config with per-job workflow agent config.
Changes:
- Add
mergeWorkflowJobAgentConfighelper to deep-merge runner config + per-job workflow config (per-job wins). - Update workflow job dispatch path to fetch the runner (
job_agent) row and marshal the merged config into the job record. - Add unit tests covering credential preservation, override precedence, and nil inputs.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| apps/workspace-engine/svc/http/server/openapi/workflows/setters.go | Fetch runner row during workflow job dispatch and merge runner config with per-job workflow config before persisting job. |
| apps/workspace-engine/svc/http/server/openapi/workflows/workflows_test.go | Adds unit tests for the merge behavior used by workflow-triggered job dispatch. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func mergeWorkflowJobAgentConfig( | ||
| runnerConfig, perJobConfig map[string]any, | ||
| ) oapi.JobAgentConfig { | ||
| return oapi.DeepMergeConfigs(runnerConfig, perJobConfig) |
| runner, err := queries.GetJobAgentByID(ctx, jobAgentIDUUID) | ||
| if err != nil { | ||
| return fmt.Errorf("get job agent runner: %w", err) | ||
| } | ||
| mergedConfig := mergeWorkflowJobAgentConfig(runner.Config, jobAgent.Config) | ||
| jobAgentConfig, err := json.Marshal(mergedConfig) |
| jobAgentConfig, err := json.Marshal(jobAgent.Config) | ||
| runner, err := queries.GetJobAgentByID(ctx, jobAgentIDUUID) | ||
| if err != nil { | ||
| return fmt.Errorf("get job agent runner: %w", err) |
Reject dispatch when the runner row's workspace doesn't match the workflow's — without it, a workflow could reference a JobAgent UUID from another workspace and pull its credentials into a job. Also tightens the merge helper to take oapi.JobAgentConfig directly.
|
Pushed 159395c addressing the Copilot review:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| jobAgentIDUUID, workspaceIDUUID, | ||
| ) | ||
| } | ||
| mergedConfig := mergeWorkflowJobAgentConfig(runner.Config, jobAgent.Config) |
| workspaceIDUUID, err := uuid.Parse(workspaceID) | ||
| if err != nil { | ||
| return fmt.Errorf("parse workspace id: %w", err) | ||
| } |
Why
Workflow runs were dropping the JobAgent runner's
serverUrl/apiKeywhen building the job, so the argo-workflow dispatcher had nothing to authenticate with. The deployment path already merges runner + deployment + version configs injobeligibility/reconcile.go— workflows took a shortcut that skipped that step.Changes
dispatchJobForAgentand merge its config with the per-jobWorkflowJobAgent.Config(per-job wins on conflict).Summary by CodeRabbit
Improvements
Tests